-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bye bye Foundation & Browserify, hello Webpack & PostCSS #77
Conversation
Interesting fact, because we have a Not sure how I feel about having two presets installed but at the other hand it does decouple our build process from the code transpiler so that's good... 🤔 |
gulp-suitcss
@thomasdegry let's move the linting modules to We can potentially remove the stylelint packages since |
- Removed `.stylelintrc` file and moved everything to a `stylelint.config.js` file so we can consume it for both `npm run lint` as well as our build process - Added some custom media queries to our `app.css` to make our linter pass without warnings - Moved all linting stuff to devDependencies and removed some older dependencies for bem support
@akrawchyk check out the changes in the last commit; that should take care of a lot of these things! |
3b61f9d
to
2eeb568
Compare
NODE_ENV: production | ||
dependencies: | ||
override: | ||
- npm install --dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thomasdegry can we use yarn
here instead? https://circleci.com/docs/1.0/yarn/
Also, can you try integrating an example application with circle before we merge this?
@thomasdegry great work, this is looking complete. Along with my comment at #77 (comment) can we also try deploying an example project to Heroku static?
|
Good ideas! Will get that done tomorrow! |
@akrawchyk Heroku deploy went smooth. Had to move As for circle, I tried using yarn instead of npm to install the dependencies but ran into this issue with this package failing:
So for now I switched circleci back to npm and the |
{{cookiecutter.repo_name}}/README.md
Outdated
To add more, uncomment the appropriate includes from the `app.scss` file along | ||
with the appropriate settings section for the component in the `_.settings.scss` | ||
file. This is described in detail at [Foundation's Sass docs](http://foundation.zurb.com/sites/docs/sass.html#adjusting-css-output). | ||
As for the JS you need to explicitely indicate which values you want passed to your code via webpack to prevent passing any sensitive data. You can do so by modifying the `webpack.config.js` file, we use the `DefinePlugin` to make our environment variables available in our code. This means you can use `process.env.SECRET_MESSAGE` in your Javascript files and the appropriate environment variable will be substituted during the build process to be shipped in the browser. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
explicitely --> explicitly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha good catch; thank you!
"babelify"{% if cookiecutter.use_foundation_sites == 'y' -%}, | ||
"browserify-shim"{%- endif %} | ||
] | ||
"browser-sync": "^2.18.8", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be using carats in front of the dependencies as it could put us at risk of drifting to different versions/breaking things? https://devcenter.heroku.com/articles/node-best-practices
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aha! We have a yarn.lock
file to avoid different versions being installed on different projects. When you do the initial project cookiecutter everything should start in the same place every time. If the devs choose to upgrade packages after that, they can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
{{cookiecutter.repo_name}}/README.md
Outdated
|
||
``` | ||
npm run dev | ||
``` npm run dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need a newline between ``` and npm run dev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Thanks
} | ||
|
||
@media(--sm-only-viewport) { | ||
color: red; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use the hex code instead?
@@ -0,0 +1,3 @@ | |||
.Foo { | |||
background-color: red; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hex code here too!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in both places
<div class="Grid Grid--fit"> | ||
<div class="Grid-cell"> | ||
<header> | ||
<h1 style="margin: 2rem 0 1rem;">The Grid</h1> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we are using inline styles here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@j-baconator yes; this css needs to live somewhere for documentation purpose so either I "pollute" the main css file; or I create another css file but this html gets deleted on npm build
. So this is just to make the grid docs look better
dateDisplayEl.innerHTML = new Date() | ||
document.body.appendChild(dateDisplayEl) | ||
|
||
console.log(SECRET_MESSAGE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shhh don't show the secret message!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙈
--font-size: 1rem; | ||
|
||
--primary-color: #43d8f8; | ||
--secondary-color: red; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
red here too!
Idea
There are a couple of things playing here but this PR has some objectives:
Todo (feel free to add or suggest)
npm run lint
package.json
and get rid of unused ones >> more or less done; just unsure if we still need/wanteslint-config-airbnb
,eslint-plugin-react
,stylelint-config-standard
andstylelint-selector-bem-pattern
README.md
file to represent the new boilerplateHave a small CSS boilerplate that has a normalize and some basic settingsWONTFIX, suitcss comes with this by default, see https://github.com/suitcss/suit/blob/master/index.css#L1 -> https://github.com/suitcss/base/blob/master/index.css#L1